Add column names method #311
Conversation
There was a problem hiding this comment.
@hatsu38 Thank you for submitting this!
If the goal here is to make sure active hash integrates with the CSV gem, can you please add a new spec that verifies this is working as expected? I'd suggest creating a new directory, spec/integration, and a new spec file.
I would also be interested in a short "how to generate CSV" in the README.md file.
|
I would also like to see a short explicit test in |
|
Rails does not have I'd feel more comfortable with just doing the work for column_names. def column_names
field_names.map(&:name)
endAlso looks like |
Using Symbol#name instead of Symbol#to_s avoids creating a new string for each invocation, improving performance and memory efficiency.
Ruby versions prior to 3.0 do not have the name method in the Symbol class.
lib/active_hash/base.rb
Outdated
| field_names.map do |field| | ||
| field.respond_to?(:name) ? field.name : field.to_s.freeze | ||
| end |
There was a problem hiding this comment.
Now field names are all symbols, so I think we can simplify this:
| field_names.map do |field| | |
| field.respond_to?(:name) ? field.name : field.to_s.freeze | |
| end | |
| fields.map(&:name) |
Does that work for you?
There was a problem hiding this comment.
After merging the latest master, the code worked perfectly! 0306858
|
@flavorjones Mike, I almost think dropping Ruby <3.0 (and therefore rails rails <=6.0. It sounds drastic but they are all EOL since early 2023, and this will work for them, just not the new OTOH/ we could use |
|
Oof, hmm. Well for the record: Rails 6.1 is still supported, and it still technically supports Ruby 2.7. Totally annoying. But: you're right that this is a new feature, and Rails 6.1 is only supported for security updates at this point. So I agree it is feasible and probably preferable to drop support for Ruby < 3.0 in the next minor release. |
|
w |
spec/active_hash/base_spec.rb
Outdated
| it "returns an array of column names" do | ||
| expect(Country.column_names).to eq(["name", "iso_name", "size"]) | ||
| end |
There was a problem hiding this comment.
@flavorjones do we want to just conditionally do this?
| it "returns an array of column names" do | |
| expect(Country.column_names).to eq(["name", "iso_name", "size"]) | |
| end | |
| it "returns an array of column names" do | |
| skip unless RUBY_VERSION >= "3.0" | |
| expect(Country.column_names).to eq(["name", "iso_name", "size"]) | |
| end |
There was a problem hiding this comment.
Hard fought. This looks good.
@flavorjones The githubs humbly requests you take a look at this too.
This pull request adds the column_names method to the ActiveHash gem.
The column_names method is analogous to the one found in Rails' ActiveRecord, allowing users to retrieve the names of columns defined in an ActiveHash model.
Added column_names method to ActiveHash::Base class.
The method returns an array of string column names, similar to ActiveRecord's column_names method.
close #310